[GLUTEN-12377][CI] Stop quarantining the Delta DV row-index failures - #12829
Open
felipepessoto wants to merge 3 commits into
Open
[GLUTEN-12377][CI] Stop quarantining the Delta DV row-index failures#12829felipepessoto wants to merge 3 commits into
felipepessoto wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Removes Delta Spark UT “quarantine by error signature” entries for the intermittent native Delta DV row-index failures, so those failures are enforced again now that the upstream Velox scan defect is fixed.
Changes:
- Remove the two DV row-index error-signature regex patterns from
flaky-error-patterns.txt. - Remove the related explanatory note from
flaky-tests.txt. - Update the Delta Spark UT utility README to mark the worked signature-quarantine example as historical and link to the upstream Velox issue.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/util/delta-spark-ut/README.md |
Updates documentation for signature-based quarantine and marks the DV example as historical with upstream references. |
.github/workflows/util/delta-spark-ut/flaky-tests.txt |
Drops the note claiming DV failures are handled via error-signature quarantine. |
.github/workflows/util/delta-spark-ut/flaky-error-patterns.txt |
Removes the two quarantined DV error signatures, leaving the file empty of patterns. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The native Delta bitmap aggregator intermittently aborted during a MERGE writing deletion vectors, with a garbage row index. Because it landed on a different *DVs*Suite test each run, it was quarantined by error signature rather than by test name. The root cause was not in the aggregator. It was a Velox scan defect: with no child readers, a filter and no deletion, 'outputRows()' was empty while the result carried numValues rows, so the row-index child came back with zero rows and downstream reads ran off the end of a zero-length buffer. Fixed upstream in facebookincubator/velox#18536, so the aggregator now receives real row indexes and the suite can be enforced again. Remove both patterns and the two cross-references that named them. The README keeps the example, marked historical, since it documents the mechanism. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The example regex allowed an optional minus sign, which contradicted both the pattern it documents and the line above it saying patterns are deliberately specific. The file's real pattern was `-\d+`, with a comment explaining that the check is `value >= 0` so the index is always negative and the sign should be matched explicitly. Also name the Velox PR that fixed the root cause and the pin that picked it up, so the historical note says where the fix landed rather than just that it did. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
felipepessoto
force-pushed
the
remove-dv-flaky-quarantine
branch
from
August 21, 2026 23:59
a9ceba2 to
9fee7d0
Compare
The regex block sits above the paragraph explaining that the patterns are no longer active, so a reader skimming for config could mistake it for the current contents of flaky-error-patterns.txt and copy it back in. Mark it inside the block, where it cannot be missed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes are proposed in this pull request?
The Delta Spark UT gate quarantines two native error signatures:
Both come from the native Delta bitmap aggregator receiving a garbage row index during a
MERGEthat writes deletion vectors. Because the abort landed on a different*DVs*Suitetest each run, matching by test name was whack-a-mole, so it was quarantined by error signature instead.The root cause was not in the aggregator, and not in Gluten. It was a Velox scan defect: in
SelectiveStructColumnReaderBase::next(), the no-child-readers branch sized the result vector tonumValuesbut sized the synthesized fields fromoutputRows(), which is empty when the scan has a filter and no deletion. The row-index child therefore came back with zero rows inside aRowVectorreporting N.BaseVector::wrapInDictionary()does not bounds-check indexes against the base, so downstream reads ran off the end of a zero-length buffer and returned whatever heap memory followed -- hence row indexes like9223372036854775807,-1and pointer-shaped values such as0xe43315c000007f00. Most were silently accepted; occasionally one failed the aggregator's bounds check and aborted the query.Fixed upstream in facebookincubator/velox#18536 (facebookincubator/velox#18535), merged as
1f971d3. The aggregator now receives real row indexes, so the suite can be enforced again.This PR removes both patterns, plus the two cross-references that named them:
flaky-error-patterns.txtflaky-tests.txt*DVs*SuiteMERGE entries are listed below" -- no longer trueREADME.mdMerge dependency -- satisfied. This needed the Velox pin to contain
1f971d3. Gluten now pinsdft-2026_08_21, which does, and this branch is rebased onto that bump, so the PR is ready to merge.Fixes #12377
How was this patch tested?
The Delta Spark UT runs on this PR:
.github/workflows/util/delta-spark-ut/**is in the workflow'spaths:filter. With the patterns removed, a DV abort is now counted as a regression instead of being dropped.A green run here is weak evidence on its own. The abort is intermittent -- that is precisely why it was quarantined by signature rather than by test name -- so the suite can pass on a given run whether or not the Velox fix is present. Red would be informative (the bug still fires); green would not prove much. The dependable check is simply whether the pinned Velox tag contains
1f971d3;dft-2026_08_21does.This PR demonstrated that caveat by accident. Its first run, before the rebase, went 8 of 8 green while building
dft-2026_08_17-- a tag that does not contain the fix. So a fully green Delta suite says nothing about whether the bug is present. The current run is against the fixed pin.The fix itself was therefore validated separately, by making the failure deterministic instead of relying on chance. Two throwaway PRs enabled Velox's
debug.validate_output_from_operatorsacross the Delta suite, which turns the malformed vector into an immediate, reproducible error:Child vector has size 0 less than parent and parent has no nulls.UPSTREAM_VELOX_PR_ID=18536: 8 of 8 shards green.That A/B, not this PR's own run, is the evidence that the row indexes were corrupt and that velox#18536 fixes them.
Upstream, the fix carries two regression tests in
TableScanTest, covering both ways the branch is reached:rowIndexWithFilterOnPartitionKeyOnly(static subfield filter) androwIndexWithDynamicFilterOnPartitionKey(no filter in the plan; a join on the partition key supplies a dynamic filter at runtime). Both fail before the fix and pass after it.Locally,
compare-test-results.pywas exercised against the edited file:load_patterns()returns[], and the signature matcher returnsFalsefor the old DV error -- i.e. such a failure is now enforced rather than ignored.Was this patch authored or co-authored using generative AI tooling?
Generated-by: GitHub Copilot CLI (Claude Opus 5)